fix #2118 - rework Node._getcustomclass and Node compat properties #2120
Conversation
RonnyPfannschmidt
changed the title
RonnyPfannschmidt
changed the title
| else: | ||
| cls = getattr(self, name) | ||
|
|
||
| warnings.warn("use of node.%s is deprecated, " |
There was a problem hiding this comment.
Isn't node.Something the only usage which has been "pending-deprecated" by _CompatProperty? AFAICT this is the only user of the _CompatProperty class, so it seems this will generate two warnings (a PendingDeprecationWarning and a DeprecationWarning?
There was a problem hiding this comment.
there is different use-cases there
one warns about using the attributes, the other warns about custom declarations while still supporting them
custom declarations will override the properties
the coode i posted ensures that
- internal usage as planned does not warn
- overriding the compatproperties will warn on internal usage
- using the properties in external code will warn
There was a problem hiding this comment.
What's a "custom declaration" in this context exactly? Subclassing Node and creating a property named Function (for example)?
There was a problem hiding this comment.
correct, py.test previously used classes and attributes in the 1.x series
basically this is code that should have created actual warnings in 2.x and be removed in 3.x
| @@ -1,7 +1,9 @@ | |||
| 3.0.6.dev0 | |||
| ========== | |||
|
|
|||
| * | |||
| * fix issue #2118 - protect against internal deprecationerrors by changing the code path of Node._getcustomclass. | |||
There was a problem hiding this comment.
Could you please add a link to #2118?
There was a problem hiding this comment.
@RonnyPfannschmidt besides adding the link, I think it needs rewording to make more sense to users (which don't know about pytest's internals). Suggestion:
- pytest no longer generates ``PendingDeprecationWarning`` from its own operations, which was introduced by mistake in version ``3.0.5`` (`#2118`_).
Thanks to `@nicoddemus`_ for the report and `@RonnyPfannschmidt`_ for the PR.
| else: | ||
| cls = getattr(self, name) | ||
|
|
||
| warnings.warn("use of node.%s is deprecated, " |
There was a problem hiding this comment.
What's a "custom declaration" in this context exactly? Subclassing Node and creating a property named Function (for example)?
|
After thinking about this a little more, I don't think we should be deprecating things in patch releases at all. Users expect only bug fixes and really minor improvements, and the fact that it is possible to use I propose we revert back the added deprecation and reapply it together with this PR to |
|
we had a "deprecation" in there since years, it just never showed - personally i would rather add them in a showing manner and do a feature release right after merging rather than doing another point release i'm really fed up with the dance of the branches |
So for most (all) users the deprecation never happened... that's part of the problem.
I disagree, I don't think we have enough features to release
IMHO it works really well. Do you disagree with using semantic versioning or the fact that we have two separate branches? Why do you have a problem with it? Do you have a different proposal? |
|
@RonnyPfannschmidt any news on the points above? IMHO we should revert the added deprecation and put it back into the |
todo: reenable in the features branch
|
Aside from the changelog minor update, LGTM, thanks! 👍 |
|
Cool, thanks! |
No description provided.